Send measure metric even when the block raises#111
Send measure metric even when the block raises#111maximebedard wants to merge 1 commit intomasterfrom
Conversation
wvanbergen
left a comment
wvanbergen
left a comment
There was a problem hiding this comment.
One issue is that calls that fail with an exception most likely have much different characteristics compared to calls that are successful. This simply combines those two histograms into one.
Maybe that makes sense, but I can also see many cases where it doesn't. What do you think?
lib/statsd/instrument.rb
Outdated
| start = Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| yield | ||
| Process.clock_gettime(Process::CLOCK_MONOTONIC) - start | ||
| def self.gettime |
There was a problem hiding this comment.
This method is part of the public API. We cannot really change it like this. I know we have code out there that uses this function
| end | ||
| else | ||
| collect_metric(hash_argument(metric_options).merge(type: :ms, name: key, value: value)) | ||
| end |
There was a problem hiding this comment.
I think you can still use the old duration method.
value = 1000 * StatsD::Instrument.duration do
begin
result = yield
ensure
...
end
endThere was a problem hiding this comment.
The problem is that we need to have the duration by the time we reach the ensure block in order to deliver the metric with the actual length of the call. I dont think this would work :(
True, I haven't considered it that way. Should we publish a different metrics in this case? |
|
TBH I'm not 100% sure what the correct behaviour is. For compairaison, datadog's library also delivers the metric when the block raises https://github.com/DataDog/dogstatsd-ruby/blob/master/lib/datadog/statsd.rb#L203-L204. For me, I believe the noise may be a good thing since it will skew metrics and potentially help pinpoint errors/unexpected behaviour. |
I see your concern and to tell the truth I might be biased by the recent work I've been doing. The way I see it I want this gem to "tell me information on what happens when I run this block" and silencing it is counter intuitive for me. (eg.: measuring around an http call might tell me all is fine but in reality a lot of requests timeout) Something else to note, other events would still track failures. eg.:
So maybe what is really missing is: That leaves us with Existing code point in the direction I want, so I'm using it as my argument. I'm open to changing how the This gets my 👍 if we are considering "making |
|
If you think this is the right way to go, I am on board. We just need to make sure to keep the |
maximebedard
force-pushed
the
measure-exceptions
branch
from
83115ac to
e29b9bc
Compare
|
This is embarassing... I just noticed you guys had this PR open... 🤦♂️ EDIT: Anyways, you guys' version looks good, so let's just get this thing rolling. 🚀 |
tjoyal
left a comment
tjoyal
left a comment
There was a problem hiding this comment.
imo 
If we do 100% agree on the direction I'm open to discuss it, but I think statsd_measure should handle exception exit path the same way as statsd_count.
I prefer this PR so 👍 for me.
If we do not want, then we fix statsd_count.
But we need to move the needle and keep the ball rolling.
If we go the route of this PR, I think it would mean we could add statsd_measure_if and statsd_measure_success to make "measure" works like "count" and "everything possible"
|
I'm sorry if I start to sound like a broken record, but when can we expect to have this merged in? 😇 Thanks guys! ❤️ |
lib/statsd/instrument.rb
Outdated
| begin | ||
| result = yield | ||
| ensure | ||
| value = 1000 * StatsD::Instrument.current_timestamp - start |
There was a problem hiding this comment.
Can you add parentheses here? 1000 * (StatsD::Instrument.current_timestamp - start). I am not too sure what the rules are in Ruby, with parentheses it's clear that it will be correct.
maximebedard
force-pushed
the
measure-exceptions
branch
from
e29b9bc to
7944487
Compare
maximebedard
force-pushed
the
measure-exceptions
branch
from
7944487 to
1f9d011
Compare
|
I've updated this PR, but I've also started building the new API here: #114 for anyone interested |
|
Hey guys, it's me again. 😊 While we don't have the final version of the new API that @maximebedard is working on, can we get this PR merged and release a new minor of the gem? This would help me a lot to get better metrics in the short to mid-term. I can also try to help on a solution for a more definitive way of doing things, if that's needed. Thanks! ❤️ |
|
I was bitten by this today as well. Can we please get this PR merged and have a minor version released? 🙏 Thank you! |
|
This has been implemented in #134 |
5 participants
Fixes #110
In order to fix this issue, I had to break down the duration method since we need to handle situation where the block would raise in order to calculate the remaining time. Since it was the only place used, I decided to expose the
gettimemethod that uses the method available to gather the current time.